Fix setup apply failure exit status#5098
Conversation
|
Thanks for the fix @mattfaltyn. The added code is counting failures for creation but not for listing. I think we should do that as well as we have couple of the |
Listing failures are counted now too, and I added tests. Thanks for catching that. |
Thanks for the quick fix. I will take a look later tonight. |
| def execute(self, api: PolarisDefaultApi) -> None: | ||
| """Execute the setup command based on the subcommand (apply or export).""" | ||
| if self.setup_subcommand == Subcommands.APPLY: | ||
| self._failure_count = 0 |
There was a problem hiding this comment.
nit: this is not necessary right as we default to 0 in the filed.
There was a problem hiding this comment.
Good point. Each CLI and REPL invocation creates a new SetupCommand, so the field default is sufficient. Removed the redundant reset in ae652a8.
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
@mattfaltyn LGTM. Thanks for the fix. Left one minor nit. |
| except Exception: | ||
| logger.warning( | ||
| f"Could not verify existence of policy '{policy_name}', attempting creation." | ||
| self._record_failure( |
There was a problem hiding this comment.
I think we could have a false failure when a policy existence check fails but creation then succeeds.
The generic exception branch of the load_policy check now calls _record_failure() while still setting policy_exits = false and proceeding to create the policy. If that load_policy error was transient and the subsequent create_policy succeeds, the resource was provisioned correctly yet the whole command still exits non-zero. Every other _record_failure() marks an operation that actually failed, but this one marks the operation that may well have succeeded.
I suggest to consider only recording the failure if the follow-up create also fails (or back to a warning).
There was a problem hiding this comment.
Good catch. In ae652a8, a real apply now logs the lookup failure as a warning and lets the follow-up create determine the result. A successful create no longer increments the failure count, while dry-run still records the lookup failure because no create occurs. I also added a regression test for the recovered path.
|
@mattfaltyn mind check on the conflict? |
ae652a8 to
b9a9bf0
Compare
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM overall. Thanks @mattfaltyn ! One test suggestion: the negative cases are well covered, but there's no explicit happy-path test asserting a fully-successful apply raises no CliError and leaves _failure_count == 0. Adding one would lock in the 'exit 0 only on full success' contract.
| logger.warning( | ||
| f"Could not verify existence of policy '{policy_name}', attempting creation." | ||
| ) | ||
| if dry_run: |
There was a problem hiding this comment.
Worth a one-line comment here on why a policy-lookup failure is counted only in dry-run. In a real apply you skip the count because the create attempt below will fail (or succeed) on its own and record its own result, but that reasoning isn't obvious from the branch alone, and the asymmetry looks like an oversight at first read. Can you add a short comment noting the create attempt covers the non-dry-run case?
b9a9bf0 to
b7964e5
Compare
|
@flyrain Thanks for the review. Both of your suggestions are now addressed in b7964e5: I added the one-line explanation for why policy lookup failures are counted only during dry-run, and added a successful setup apply regression test that verifies _failure_count == 0 and that the requested role is created. The focused setup command suite passes (12 tests). |
Thanks for the fix. I will take a look later tonight. |
4 participants
Summary
setup apply.Why
setup applycatches setup API exceptions to continue applying the rest of aconfiguration. Previously, those caught failures were never reflected in the
command result, so automation received exit status 0 and a success message even
when one or more requested resources were not provisioned. Discovery failures
could also be cached as empty results, causing later work to be skipped without
failing the command.
The command now counts those failures and raises a
CliErrorafter processingthe configuration. Regression tests verify the nonzero result, best-effort
continuation, discovery-failure handling (including policy lookups), invalid
external-catalog configuration, unresolved role assignments, and the expected
dry-run 404 behavior.
Fixes #5097
Testing
make client-unit-test(159 tests passed on Python 3.14.6)PRE_COMMIT_HOME=/tmp/polaris-pre-commit make client-lint(all format, Ruff, and mypy hooks passed)make client-license-check(passed)JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@21/bin:$PATH ./gradlew format compileAll(build successful)JAVA_HOME=/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@21/bin:$PATH ./gradlew check(attempted; unrelated Docker-backedpolaris-adminand Azurite tests could not start because Docker is not installed locally)check sourceTarball distTar distZip publishToMavenLocal -x :polaris-runtime-service:test -x :polaris-admin:test -PnoIntegrationTests --continue) completed all 1,887 tasks except the Docker-backed:polaris-relational-jdbc:test, whose Testcontainers PostgreSQL initialization could not start without a local Docker socket.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(not needed; CLI syntax and configuration are unchanged)